Add FXIOS-14493 #31366 [Onboarding] No telemetry recorded when the user taps the "go to settings" button#4
Open
Add FXIOS-14493 #31366 [Onboarding] No telemetry recorded when the user taps the "go to settings" button#4
Conversation
- Extract FxNimbus TOS feature configuration into setTermsOfServiceFeatureEnabled helper - Set TOS feature to false in setUp() to ensure clean test state - Update testLaunchType_termsOfService to use helper method - Improves maintainability: only helper needs updating if YAML config changes
- Move go_to_settings_pressed and dismiss_pressed metrics from default_browser_onboarding to onboarding section - Update TelemetryWrapper to use new onboarding metrics - Add sendGoToSettingsTelemetry() and sendDismissPressedTelemetry() to OnboardingTelemetryUtility - Update modern onboarding (OnboardingService) to record telemetry when setDefaultBrowser/openIosFxSettings actions are triggered - Update legacy onboarding (IntroViewController) to use telemetry utility - Update DefaultBrowserOnboardingViewController to use telemetry utility - Add hasDefaultBrowserCard() helper method to OnboardingService - Record dismiss_pressed when modern onboarding with default browser cards is dismissed Fixes mozilla-mobile#31366
- Add onDismiss closure to OnboardingBottomSheetViewController - Record dismiss_pressed telemetry when default browser popup close button is tapped - Record dismiss_pressed when dismissing onboarding flow containing default browser cards
…nboarding metrics
- Record go_to_settings_pressed when user taps Go to Settings button - Record dismiss_pressed when user dismisses the popup via button or close button - Add telemetry in buttonTappedFinishFlow callback for both modern and legacy onboarding flows - Make OnboardingBottomSheetViewController implement BottomSheetDelegate - Add onDismiss closure to OnboardingBottomSheetViewController for close button telemetry
- Record go_to_settings_pressed when user taps Go to Settings button - Record dismiss_pressed when user dismisses the popup via button or close button - Add telemetry in buttonTappedFinishFlow callback for both modern and legacy onboarding flows - Make OnboardingBottomSheetViewController implement BottomSheetDelegate - Add onDismiss closure to OnboardingBottomSheetViewController for close button telemetry
- Remove code that recorded dismiss_pressed when dismissing onboarding flow - Remove unused hasDefaultBrowserCard function from OnboardingService
- Remove code that recorded dismiss_pressed when dismissing onboarding flow - Remove unused hasDefaultBrowserCard function from OnboardingService
…metrics - Remove old bugs and data_reviews links - Keep only issue mozilla-mobile#31366 in bugs - Keep only PR mozilla-mobile#31375 in data_reviews
- Remove telemetryUtility dependency from DefaultBrowserOnboardingViewController - Use TelemetryWrapper.recordEvent directly for dismiss and go-to-settings actions - Add new EventObject cases: dismissDefaultBrowserOnboarding and goToSettingsDefaultBrowserOnboarding - Map new events to onboarding.dismissPressed and onboarding.goToSettingsPressed metrics - Update LaunchCoordinator to remove telemetryUtility creation
… tests - Make telemetryUtility required (non-optional) in OnboardingService - Remove optional chaining when calling telemetry methods - Add MockOnboardingTelemetryUtility for testing - Add comprehensive unit tests for sendGoToSettingsTelemetry() and sendDismissPressedTelemetry() - Update test setup to use setupTelemetry/tearDownTelemetry for Glean initialization - Test coverage includes single calls, multiple calls, and different onboarding variants
- Revert TelemetryWrapper.recordEvent calls to single-line format - Revert DefaultBrowserOnboardingViewController initialization to single-line format - Move new telemetry cases to end of Default Browser section to preserve original line positions
…r weak - Changed qrCodeNavigationHandler from strong (let) to weak (var) in OnboardingService - This breaks the retain cycle: LaunchCoordinator -> onboardingService -> qrCodeNavigationHandler (strong) -> LaunchCoordinator - Updated LaunchCoordinator to properly handle onboardingService with weak captures in closures - Clear onboardingService when onboarding completes or is dismissed
…metrics - Restore issue mozilla-mobile#7870 in bugs for go_to_settings_pressed and dismiss_pressed - Restore PRs mozilla-mobile#7245, mozilla-mobile#9673, mozilla-mobile#12334, mozilla-mobile#14102 in data_reviews for both metrics - Keep the latest links (mozilla-mobile#31366 and mozilla-mobile#31375) that were added
…browser-onboarding-telemetry
…_pressed metrics These metrics were replaced by onboarding.dismiss_pressed and onboarding.go_to_settings_pressed. Removed the old EventObject cases and their switch handlers in TelemetryWrapper.
… onboarding_variant - Changed from counter metrics to event metrics under onboarding.default_browser_sheet - Added onboarding_variant extra key to track flow type (legacy, modern, japan) - Updated metric names to match new standards: go_to_settings_button_tapped and dismiss_button_tapped - Updated function names to match: sendGoToSettingsButtonTappedTelemetry and sendDismissButtonTappedTelemetry - Updated all Swift code to use GleanMetrics.OnboardingDefaultBrowserSheet (dot notation pattern) - Updated tests to use mock pattern consistent with other onboarding telemetry tests
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| buttonTappedFinishFlow: { [weak self] in | ||
| self?.telemetryUtility?.sendGoToSettingsButtonTappedTelemetry() | ||
| completion() | ||
| } |
There was a problem hiding this comment.
Missing dismiss telemetry in OnboardingService popup
Medium Severity
The createDefaultBrowserPopupViewController method in OnboardingService does not set the bottomSheetVC.onDismiss callback to record dismiss telemetry. In contrast, OnboardingCardDelegate.presentDefaultBrowserPopup properly sets bottomSheetVC.onDismiss to call sendDismissButtonTappedTelemetry(). This means dismiss button taps in the modern onboarding flow (which uses OnboardingService) won't be recorded, defeating part of the PR's purpose to add telemetry for dismiss actions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Benchmark PR from qodo-benchmark#24
Note
Introduces explicit telemetry for the default-browser bottom sheet and integrates it across onboarding.
onboarding.default_browser_sheet.{go_to_settings_button_tapped,dismiss_button_tapped}events inonboarding.yaml; removes obsolete counters frommetrics.yamland updatesTelemetryWrapperto record new events (legacy variant)sendGoToSettingsButtonTappedTelemetryandsendDismissButtonTappedTelemetryinOnboardingTelemetryUtilityand extendsOnboardingTelemetryProtocolIntroViewController, popup completion inOnboardingService, instructions popup and sheet dismiss inOnboardingCardDelegate(via newOnboardingBottomSheetViewController.onDismiss)LaunchCoordinatorto ownOnboardingServiceinstance, assigns telemetry utility, and clears it on completion/dismiss; makes QR code handler weak inOnboardingServiceWritten by Cursor Bugbot for commit 57c6363. Configure here.